Open
Conversation
Partially borrowed from Nixpkgs Signed-off-by: NotAShelf <raf@notashelf.dev> Change-Id: Icae2926727255621cc1178488f7b65f06a6a6964
Signed-off-by: NotAShelf <raf@notashelf.dev> Change-Id: I4fc35496a845d9a16179a20b831a22526a6a6964
WalkthroughAdds a cross-system NixOS VM test (nh-remote-test), test SSH keys, and CI changes to run the new test; flake.nix is updated to expose per-system Changes
Sequence Diagram(s)sequenceDiagram
participant CI as CI
participant Deployer as Deployer VM
participant BuildHost as BuildHost VM
participant Target as Target VM
CI->>Deployer: start nh-remote-test (nixosTest)
Deployer->>Target: provision SSH keys & known_hosts
Deployer->>BuildHost: provision SSH keys & known_hosts
Note right of Deployer: generate hardware config, transfer files
Deployer->>Target: scp hardware-configuration.nix + configs
Deployer->>Target: ssh run local build & activation
Deployer->>BuildHost: trigger remote build (--build-host)
BuildHost->>Target: build artefacts transferred or targeted deployment
Target->>Deployer: report activation status / hostname checks
Deployer->>CI: test results (pass/fail)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@flake.nix`:
- Around line 29-37: The current checks assignment uses the shallow merge
operator (//) which causes self.packages.${system} to be overwritten by the
genAttrs result and drops Linux package checks; update the merge to perform a
deep/recursive merge so nested keys are preserved — for example use
nixpkgs.lib.recursiveUpdate (or nixpkgs.lib.recursiveUpdateAttrs) to combine
self.packages and the genAttrs result that defines nh-remote-test, or refactor
to forAllSystems with conditional logic that adds nh-remote-test only for
non-Darwin systems; locate the checks assignment and replace the top-level //
merge of self.packages and the genAttrs block (the nh-remote-test definition)
with a recursiveUpdate-based merge or a forAllSystems conditional so nh and
default entries in self.packages.${system} are not lost.
In `@test/vm/test-remote.nix`:
- Around line 209-224: The remote subtests call deployer.succeed("nh os switch
... -f '<nixpkgs/nixos>'") but never create /root/configuration.nix on the
deployer; copy the appropriate prepared file to /root/configuration.nix on the
deployer before each remote deployer.succeed invocation (e.g. copy
configuration-2.nix before the subtest that expects config-2-deployed and
configuration-3.nix before the subtest that expects config-3-deployed). Use
deployer.succeed to perform the copy on the deployer host immediately prior to
the deployer.succeed("nh os switch ...") calls so the switch command finds
/root/configuration.nix.
🧹 Nitpick comments (2)
.github/workflows/test.yaml (1)
6-24: Workflow paths filter may be incomplete.The paths filter references
.github/workflows/build.yaml(line 13, 24) but this file istest.yaml. Additionally, changes to test files (test/**) won't trigger this workflow, which means modifications totest/vm/test-remote.nixortest/ssh-keys.nixwon't automatically run the tests.Consider updating the paths filter:
Suggested paths additions
paths: - "Cargo.toml" - "Cargo.lock" - "xtask/Cargo.toml" - "src/**" - "xtask/src/**" - ".cargo/**" - - ".github/workflows/build.yaml" + - ".github/workflows/test.yaml" + - "test/**"Apply to both
pull_requestandpushsections.test/vm/test-remote.nix (1)
13-16: Consider documenting the security tradeoff in the SSH config.The SSH config disables host key verification, which is appropriate for ephemeral test VMs. However, the test also populates
known_hosts(lines 176-177), making these settings partially redundant. Consider either:
- Removing
known_hostspopulation since verification is disabled, or- Removing the
StrictHostKeyChecking=nosinceknown_hostsis populatedThe current approach works but is slightly inconsistent.
Comment on lines
+29
to
+37
| checks = | ||
| self.packages | ||
| // self.devShells | ||
| // nixpkgs.lib.genAttrs [ "x86_64-linux" "aarch64-linux" ] (system: { | ||
| nh-remote-test = nixpkgs.legacyPackages.${system}.callPackage ./test/vm/test-remote.nix { | ||
| inherit (self.packages.${system}) nh; | ||
| inherit nixpkgs; | ||
| }; | ||
| }); |
There was a problem hiding this comment.
Incorrect attribute set merge overwrites checks for Linux systems.
The // operator performs shallow merging at the top level (system keys), not a deep merge of the inner attributes. This means:
self.packages.x86_64-linux(containingnh,default) gets overwritten by thegenAttrsresult{ nh-remote-test = ...; }- Linux systems lose their package checks entirely
- Darwin systems won't have
nh-remote-test(correct) but the merge still has issues
To properly merge nested attrsets, use lib.recursiveUpdate or restructure:
Proposed fix using recursiveUpdate
checks =
- self.packages
- // self.devShells
- // nixpkgs.lib.genAttrs [ "x86_64-linux" "aarch64-linux" ] (system: {
- nh-remote-test = nixpkgs.legacyPackages.${system}.callPackage ./test/vm/test-remote.nix {
- inherit (self.packages.${system}) nh;
- inherit nixpkgs;
- };
- });
+ nixpkgs.lib.recursiveUpdate
+ (nixpkgs.lib.recursiveUpdate self.packages self.devShells)
+ (nixpkgs.lib.genAttrs [ "x86_64-linux" "aarch64-linux" ] (system: {
+ nh-remote-test = nixpkgs.legacyPackages.${system}.callPackage ./test/vm/test-remote.nix {
+ inherit (self.packages.${system}) nh;
+ inherit nixpkgs;
+ };
+ }));Alternatively, use forAllSystems with conditional logic for a cleaner approach.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| checks = | |
| self.packages | |
| // self.devShells | |
| // nixpkgs.lib.genAttrs [ "x86_64-linux" "aarch64-linux" ] (system: { | |
| nh-remote-test = nixpkgs.legacyPackages.${system}.callPackage ./test/vm/test-remote.nix { | |
| inherit (self.packages.${system}) nh; | |
| inherit nixpkgs; | |
| }; | |
| }); | |
| checks = | |
| nixpkgs.lib.recursiveUpdate | |
| (nixpkgs.lib.recursiveUpdate self.packages self.devShells) | |
| (nixpkgs.lib.genAttrs [ "x86_64-linux" "aarch64-linux" ] (system: { | |
| nh-remote-test = nixpkgs.legacyPackages.${system}.callPackage ./test/vm/test-remote.nix { | |
| inherit (self.packages.${system}) nh; | |
| inherit nixpkgs; | |
| }; | |
| })); |
🤖 Prompt for AI Agents
In `@flake.nix` around lines 29 - 37, The current checks assignment uses the
shallow merge operator (//) which causes self.packages.${system} to be
overwritten by the genAttrs result and drops Linux package checks; update the
merge to perform a deep/recursive merge so nested keys are preserved — for
example use nixpkgs.lib.recursiveUpdate (or nixpkgs.lib.recursiveUpdateAttrs) to
combine self.packages and the genAttrs result that defines nh-remote-test, or
refactor to forAllSystems with conditional logic that adds nh-remote-test only
for non-Darwin systems; locate the checks assignment and replace the top-level
// merge of self.packages and the genAttrs block (the nh-remote-test definition)
with a recursiveUpdate-based merge or a forAllSystems conditional so nh and
default entries in self.packages.${system} are not lost.
Comment on lines
+209
to
+224
| with subtest("Remote build on deployer, deploy to target with --target-host"): | ||
| deployer.succeed(f"nh os switch --bypass-root-check -f '<nixpkgs/nixos>' --target-host root@{target_ip}") | ||
|
|
||
| # Verify hostname changed | ||
| target_hostname = target.succeed("cat /etc/hostname").strip() | ||
| assert target_hostname == "config-2-deployed", f"Expected 'config-2-deployed', got '{target_hostname}'" | ||
|
|
||
| # Build on buildHost, activate on target (both different from deployer) | ||
| with subtest("Remote build on buildHost with --build-host, deploy to target with --target-host"): | ||
| deployer.succeed( | ||
| f"nh os switch --bypass-root-check -f '<nixpkgs/nixos>' --build-host root@{build_host_ip} --target-host root@{target_ip}" | ||
| ) | ||
|
|
||
| # Verify hostname changed | ||
| target_hostname = target.succeed("cat /etc/hostname").strip() | ||
| assert target_hostname == "config-3-deployed", f"Expected 'config-3-deployed', got '{target_hostname}'" |
There was a problem hiding this comment.
Missing configuration file setup for remote subtests.
The remote build subtests (lines 209-248) run nh os switch with -f '<nixpkgs/nixos>' on the deployer, which expects /root/configuration.nix to exist. However, only /root/configuration-{1,2,3}.nix files are copied to the deployer (lines 189-191), and /root/configuration.nix is never created on the deployer.
Each remote subtest should copy the appropriate configuration file to /root/configuration.nix on the deployer before running the switch command:
Proposed fix for the second subtest
# Build on deployer, activate on target
with subtest("Remote build on deployer, deploy to target with --target-host"):
+ deployer.succeed("cp /root/configuration-2.nix /root/configuration.nix")
deployer.succeed(f"nh os switch --bypass-root-check -f '<nixpkgs/nixos>' --target-host root@{target_ip}")Similar changes needed for the other remote subtests with the appropriate configuration file.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| with subtest("Remote build on deployer, deploy to target with --target-host"): | |
| deployer.succeed(f"nh os switch --bypass-root-check -f '<nixpkgs/nixos>' --target-host root@{target_ip}") | |
| # Verify hostname changed | |
| target_hostname = target.succeed("cat /etc/hostname").strip() | |
| assert target_hostname == "config-2-deployed", f"Expected 'config-2-deployed', got '{target_hostname}'" | |
| # Build on buildHost, activate on target (both different from deployer) | |
| with subtest("Remote build on buildHost with --build-host, deploy to target with --target-host"): | |
| deployer.succeed( | |
| f"nh os switch --bypass-root-check -f '<nixpkgs/nixos>' --build-host root@{build_host_ip} --target-host root@{target_ip}" | |
| ) | |
| # Verify hostname changed | |
| target_hostname = target.succeed("cat /etc/hostname").strip() | |
| assert target_hostname == "config-3-deployed", f"Expected 'config-3-deployed', got '{target_hostname}'" | |
| with subtest("Remote build on deployer, deploy to target with --target-host"): | |
| deployer.succeed("cp /root/configuration-2.nix /root/configuration.nix") | |
| deployer.succeed(f"nh os switch --bypass-root-check -f '<nixpkgs/nixos>' --target-host root@{target_ip}") | |
| # Verify hostname changed | |
| target_hostname = target.succeed("cat /etc/hostname").strip() | |
| assert target_hostname == "config-2-deployed", f"Expected 'config-2-deployed', got '{target_hostname}'" | |
| # Build on buildHost, activate on target (both different from deployer) | |
| with subtest("Remote build on buildHost with --build-host, deploy to target with --target-host"): | |
| deployer.succeed( | |
| f"nh os switch --bypass-root-check -f '<nixpkgs/nixos>' --build-host root@{build_host_ip} --target-host root@{target_ip}" | |
| ) | |
| # Verify hostname changed | |
| target_hostname = target.succeed("cat /etc/hostname").strip() | |
| assert target_hostname == "config-3-deployed", f"Expected 'config-3-deployed', got '{target_hostname}'" |
🤖 Prompt for AI Agents
In `@test/vm/test-remote.nix` around lines 209 - 224, The remote subtests call
deployer.succeed("nh os switch ... -f '<nixpkgs/nixos>'") but never create
/root/configuration.nix on the deployer; copy the appropriate prepared file to
/root/configuration.nix on the deployer before each remote deployer.succeed
invocation (e.g. copy configuration-2.nix before the subtest that expects
config-2-deployed and configuration-3.nix before the subtest that expects
config-3-deployed). Use deployer.succeed to perform the copy on the deployer
host immediately prior to the deployer.succeed("nh os switch ...") calls so the
switch command finds /root/configuration.nix.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/test.yaml:
- Around line 49-51: The workflow's path filters currently omit flake.nix and
the VM tests and mistakenly reference .github/workflows/build.yaml, so PRs that
only change the flake or test/vm/ won't trigger the test workflow; update the
on: paths/pull_request (and push if present) in .github/workflows/test.yaml to
include "flake.nix" and "test/vm/**" (or "test/vm/") and correct the referenced
workflow path from ".github/workflows/build.yaml" to
".github/workflows/test.yaml" so the NH VM test step (the "Test NH features in
NixOS VM" step that builds .#checks.x86_64-linux.nh-remote-test) runs for
relevant changes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 18f77d44-14fe-4b2f-9bc7-592549b491ef
📒 Files selected for processing (1)
.github/workflows/test.yaml
Comment on lines
+49
to
+51
| - name: Test NH features in NixOS VM | ||
| run: | | ||
| nix build .#checks.x86_64-linux.nh-remote-test |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Show whether this workflow is triggered by VM/flake/test workflow changes.
sed -n '3,24p' .github/workflows/test.yaml
printf '\nExpected these paths to be present for the new VM check:\n'
rg -n 'flake\.nix|flake\.lock|test/vm|\.github/workflows/test\.yaml' .github/workflows/test.yaml || trueRepository: nix-community/nh
Length of output: 601
Include flake and VM test paths in workflow triggers so the new test actually runs.
The new step at line 51 depends on flake.nix and test/vm/ changes, but the workflow's path filters omit them. PRs that only modify the flake check or VM tests will skip this workflow entirely. Additionally, the path filter currently references .github/workflows/build.yaml but should reference .github/workflows/test.yaml.
Path-filter update needed
on:
workflow_dispatch:
pull_request:
paths:
- "Cargo.toml"
- "Cargo.lock"
+ - "flake.nix"
+ - "flake.lock"
- "xtask/Cargo.toml"
- "crates/**"
+ - "test/vm/**"
- "xtask/src/**"
- ".cargo/**"
- - ".github/workflows/build.yaml"
+ - ".github/workflows/test.yaml"
push:
branches-ignore:
- "update-*"
paths:
- "Cargo.toml"
- "Cargo.lock"
+ - "flake.nix"
+ - "flake.lock"
- "xtask/Cargo.toml"
- "crates/**"
+ - "test/vm/**"
- "xtask/src/**"
- ".cargo/**"
- - ".github/workflows/build.yaml"
+ - ".github/workflows/test.yaml"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/test.yaml around lines 49 - 51, The workflow's path
filters currently omit flake.nix and the VM tests and mistakenly reference
.github/workflows/build.yaml, so PRs that only change the flake or test/vm/
won't trigger the test workflow; update the on: paths/pull_request (and push if
present) in .github/workflows/test.yaml to include "flake.nix" and "test/vm/**"
(or "test/vm/") and correct the referenced workflow path from
".github/workflows/build.yaml" to ".github/workflows/test.yaml" so the NH VM
test step (the "Test NH features in NixOS VM" step that builds
.#checks.x86_64-linux.nh-remote-test) runs for relevant changes.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Supersedes #516
we test those
Summary by CodeRabbit
Tests
Chores